Implement first step of cancellation: Stop all handler when one completes/fails - #204
Conversation
10fb382 to
e2c3acf
Compare
There was a problem hiding this comment.
Pull request overview
Introduces shared cancellation across credential transports so remaining handlers stop when one completes, fails, or is cancelled.
Changes:
- Propagates cancellation tokens through USB, NFC, hybrid, and UI handlers.
- Completes requests on transport success or failure.
- Adds cancellation-focused tests and dependencies.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
credentialsd/src/dbus/flow_control.rs |
Stops UI event handling on cancellation. |
credentialsd/src/credential_service/mod.rs |
Coordinates cancellation and request completion. |
credentialsd/src/credential_service/usb.rs |
Adds USB cancellation handling. |
credentialsd/src/credential_service/nfc.rs |
Adds NFC cancellation handling. |
credentialsd/src/credential_service/hybrid.rs |
Adds hybrid cancellation handling. |
credentialsd/Cargo.toml |
Adds cancellation and test dependencies. |
Cargo.lock |
Locks the added dependencies. |
Suppressed comments (1)
credentialsd/src/credential_service/mod.rs:1021
- This assertion is probabilistic: two independent random
u32request IDs can legally be equal, making the test flaky. Either remove this test or inject a deterministic ID generator and test an actual uniqueness contract.
// IDs should be different (random)
assert_ne!(
request_id_1, request_id_2,
"Sequential requests should (almost certainly) have different IDs"
);
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
e2c3acf to
d5e4220
Compare
|
FYI, I pushed some cancellation stuff I had worked on before but didn't finalize to the |
|
I hope to review this weekend, but could you add a CHANGELOG entry? I'm trying to get into the habit of doing that again so releases are faster. |
iinuwa
left a comment
iinuwa
left a comment
There was a problem hiding this comment.
Left some comments. Overall looking good, and the tests are very helpful! 😁
I think in general with long tokio::select! branches, I prefer to name the futures and then select over them, or better yet, since we're only using these for concellation, to await the CancellationToken::run_until_cancelled_owned() method (Cf. here for a short example). This reduces indentation and makes the control flow a little bit clearer. (I think that would also make the diffs a little smaller too.)
I think we should talk a little bit about our plan for what happens after this lands to enable the UI to restart. We can do that in #172 or on matrix.
…ed() instead of select
d5e4220 to
ca6847c
Compare
iinuwa
left a comment
iinuwa
left a comment
There was a problem hiding this comment.
I haven't run this, but the code looks good to me! Thanks!
|
@msirringhaus If I don't get to testing this tonight, feel free to merge this in |
Again, based on #135 . Only the last commit is relevant. Will be rebased, once the other PR lands.
Next step would be cancellation events from the UI.